-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix overdependence on start/end lines in diff strategy #2567
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
0a95a49 to
4f6c5df
Compare
4f6c5df to
08d803f
Compare
3adb863 to
842bb2b
Compare
| const replacements = matches | ||
| .map((match) => ({ | ||
| startLine: Number(match[2] ?? 0), | ||
| endLine: Number(match[4] ?? resultLines.length), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not using the passed in endLine anymore (just taking the start line plus the length of the search block), but I figure we can clean that up as a follow-up if this doesn't cause any unforeseen problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cte
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me!
I think this means it can no longer modify an empty file. maybe we do not care and that should be the responsibility of write_to_file, but heads up ... |
|
Thanks yeah, that’s what I was thinking |
Fixes #2556 by using start line as a hint for where to start searching for diffs, versus previously looking for the chunk specified by start and end line. I think this could have caused incorrectly applied diffs in lots of cases depending on how the LLM interpreted the end line parameter.
This also removes the ability to pass in an empty search block, since we have no way to verify that the line number from the LLM is correct. The model should always be required to pass in a search block.
Important
Disallow empty search content (insertions) in
MultiSearchReplaceDiffStrategy, remove related tests, and add a fuzzy matching test for issue #2556.multi-search-replace.ts:MultiSearchReplaceDiffStrategy.applyDiff(). Now returns an error if search content is empty, removing support for insertions via empty search blocks.getSimilarity()to return 0 for empty search content (was 1).multi-search-replace.test.ts:This description was created by
for 842bb2b. It will automatically update as commits are pushed.